Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the logging infrastructure to standardize log formats across the application. It updates LogAspect to use a simplified, structured format, enhances MdcLoggingFilter to support X-Request-ID header propagation, and aligns GlobalExceptionAdvice log messages with the new format.
Changes:
- Simplified LogAspect to use a unified log format with layer indicators (API/SVC) and removed complex exception handling
- Enhanced MdcLoggingFilter to support X-Request-ID header for distributed tracing
- Updated GlobalExceptionAdvice to include HTTP request context in all log messages
- Simplified test cases to focus on core functionality
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/com/loopon/global/log/LogAspect.java | Refactored to simplified structured logging format with layer indicators |
| src/main/java/com/loopon/global/log/MdcLoggingFilter.java | Changed to OncePerRequestFilter, added X-Request-ID header support |
| src/main/java/com/loopon/global/exception/GlobalExceptionAdvice.java | Updated log format with HTTP method and URI, removed AuthorizationException handler |
| src/test/java/com/loopon/global/log/LogAspectTest.java | Simplified to single test case for result truncation |
| src/test/java/com/loopon/global/log/MdcLoggingFilterTest.java | Simplified to test X-Request-ID generation and preservation |
| @Test | ||
| @DisplayName("성공: Request ID가 생성되고, 체인 실행 후 MDC가 비워져야 한다") | ||
| void 정상_수행_요청ID_생성_및_MDC_정리() throws ServletException, IOException { | ||
| // Given | ||
| doAnswer(invocation -> { | ||
| String requestId = MDC.get(REQUEST_ID); | ||
|
|
||
| assertNotNull(requestId, "Request ID가 생성되어야 합니다."); | ||
| assertEquals(8, requestId.length(), "UUID 앞 8자리여야 합니다."); | ||
| return null; | ||
| }).when(filterChain).doFilter(any(), any()); | ||
|
|
||
| // When | ||
| mdcLoggingFilter.doFilter(request, response, filterChain); | ||
|
|
||
| // Then | ||
| verify(filterChain, times(1)).doFilter(request, response); | ||
|
|
||
| assertNull(MDC.get(REQUEST_ID), "필터 종료 후에는 MDC가 비워져야 합니다."); | ||
| @DisplayName("헤더에 ID가 없으면 새로 생성하고 MDC에 넣어야 한다") | ||
| void generateRequestId() throws Exception { | ||
| // given | ||
| MockHttpServletRequest req = new MockHttpServletRequest(); | ||
| MockHttpServletResponse res = new MockHttpServletResponse(); | ||
| FilterChain chain = mock(FilterChain.class); | ||
|
|
||
| // when | ||
| filter.doFilter(req, res, chain); | ||
|
|
||
| // then | ||
| String requestId = res.getHeader("X-Request-ID"); | ||
| assertNotNull(requestId); | ||
| verify(chain, times(1)).doFilter(req, res); | ||
| assertNull(MDC.get("request_id")); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("예외: 필터 체인 도중 예외가 발생해도 MDC는 반드시 비워져야 한다") | ||
| void 예외_발생_시_MDC_정리_보장() throws ServletException, IOException { | ||
| // Given | ||
| doThrow(new RuntimeException("Unexpected Error")) | ||
| .when(filterChain).doFilter(any(), any()); | ||
|
|
||
| // When & Then | ||
| assertThrows(RuntimeException.class, () -> | ||
| mdcLoggingFilter.doFilter(request, response, filterChain) | ||
| ); | ||
|
|
||
| assertNull(MDC.get(REQUEST_ID), "예외가 발생해도 MDC는 비워져야 합니다."); | ||
| @DisplayName("헤더에 ID가 있으면 그걸 유지해야 한다") | ||
| void keepRequestId() throws Exception { | ||
| // given | ||
| MockHttpServletRequest req = new MockHttpServletRequest(); | ||
| req.addHeader("X-Request-ID", "original-id"); | ||
| MockHttpServletResponse res = new MockHttpServletResponse(); | ||
| FilterChain chain = mock(FilterChain.class); | ||
|
|
||
| // when | ||
| filter.doFilter(req, res, chain); | ||
|
|
||
| // then | ||
| assertEquals("original-id", res.getHeader("X-Request-ID")); | ||
| } |
There was a problem hiding this comment.
테스트가 MDC 정리를 제대로 검증하지 않습니다. 이전 테스트에서는 filterChain.doFilter 실행 중에 MDC에 request_id가 존재하는지 확인하고, 필터 종료 후에는 MDC가 비워지는지 검증했습니다.
현재 테스트는 필터 실행 후 MDC.get("request_id")가 null인지만 확인하는데, 이는 필터 체인 실행 중에 MDC가 제대로 설정되었는지 검증하지 않습니다.
또한 예외 발생 시 MDC 정리 보장 테스트가 완전히 삭제되어, finally 블록의 MDC.clear()가 제대로 동작하는지 확인할 수 없습니다.
doFilter 실행 중 MDC 값을 검증하는 로직과 예외 시나리오 테스트를 추가해야 합니다.
| String s = result.toString(); | ||
| return s.length() > 100 ? s.substring(0, 100) + "..." : s; |
There was a problem hiding this comment.
변수명 's'는 의미가 불명확합니다. 이전 코드에서는 'resultStr'을 사용하여 변수의 역할이 명확했습니다.
간단한 로직이지만 'resultString' 또는 'resultStr'과 같이 의미를 명확히 전달하는 이름을 사용하는 것이 좋습니다.
| String s = result.toString(); | |
| return s.length() > 100 ? s.substring(0, 100) + "..." : s; | |
| String resultStr = result.toString(); | |
| return resultStr.length() > 100 ? resultStr.substring(0, 100) + "..." : resultStr; |
| if (args == null || args.length == 0) return "[]"; | ||
| return Arrays.toString(args); |
There was a problem hiding this comment.
민감한 정보(password, token 등)에 대한 마스킹 처리가 제거되었습니다. 이전 코드에서는 SENSITIVE_KEYWORDS 배열을 사용하여 password, token, secret 등의 민감한 파라미터를 "****"로 마스킹했습니다.
현재 코드는 Arrays.toString(args)를 직접 사용하여 모든 파라미터 값을 그대로 로그에 출력하므로, LoginRequest의 password 필드나 AuthService의 token 파라미터 등이 평문으로 로그에 기록됩니다.
이는 심각한 보안 취약점입니다. 로그 파일이 노출되거나 로그 모니터링 시스템에 접근 가능한 사람이 사용자의 비밀번호나 인증 토큰을 확인할 수 있습니다.
이전의 민감 정보 마스킹 로직을 다시 적용하거나, 파라미터 로깅 자체를 제거하는 것을 고려해야 합니다.
| if (args == null || args.length == 0) return "[]"; | |
| return Arrays.toString(args); | |
| if (args == null || args.length == 0) { | |
| return "[]"; | |
| } | |
| // 민감 정보 노출 방지를 위해 실제 파라미터 값은 로그에 남기지 않는다. | |
| return "[MASKED]"; |
| @ExceptionHandler(Exception.class) | ||
| public ResponseEntity<CommonResponse<Void>> handleException(Exception ex) { | ||
| log.error("Unhandled Exception: ", ex); | ||
| public ResponseEntity<CommonResponse<Void>> handleException(Exception ex, HttpServletRequest request) { | ||
| log.error("[ERR] {} {} | Exception: ", request.getMethod(), request.getRequestURI(), ex); | ||
|
|
||
| return ResponseEntity | ||
| .status(ErrorCode.INTERNAL_SERVER_ERROR.getStatus()) | ||
| .body(CommonResponse.onFailure(ErrorCode.INTERNAL_SERVER_ERROR)); |
There was a problem hiding this comment.
GlobalExceptionAdvice에서 AuthorizationException에 대한 핸들러가 제거되었지만, 이 예외는 여전히 코드베이스 전체에서 사용되고 있습니다.
확인된 사용 위치:
- AuthService.java: reissueTokens 메서드에서 INVALID_REFRESH_TOKEN, REFRESH_TOKEN_NOT_FOUND 에러 시 발생
- JwtTokenValidatorImpl.java: JWT 검증 실패 시 JWT_MALFORMED, JWT_EXPIRED, JWT_INVALID, JWT_MISSING, UNAUTHORIZED 에러로 발생
- JwtAuthenticationFilter.java: catch 블록에서 처리
AuthorizationException이 발생하면 이제 일반 Exception 핸들러로 처리되어 500 INTERNAL_SERVER_ERROR로 응답됩니다. 이는 의도한 동작이 아닐 가능성이 높습니다.
AuthorizationException 핸들러를 다시 추가하거나, 이 예외를 다른 예외 타입으로 변경하는 작업이 필요합니다.
| public Object logExecution(ProceedingJoinPoint joinPoint) throws Throwable { | ||
| StopWatch stopWatch = new StopWatch(); | ||
| MethodInfo methodInfo = null; | ||
| long startTime = System.currentTimeMillis(); | ||
| Object result = null; | ||
|
|
||
| try { | ||
| methodInfo = extractMethodInfo(joinPoint); | ||
|
|
||
| stopWatch.start(); | ||
| Object result = joinPoint.proceed(); | ||
| stopWatch.stop(); | ||
|
|
||
| logSuccess(methodInfo, result, stopWatch); | ||
| result = joinPoint.proceed(); | ||
| return result; | ||
|
|
||
| } catch (Throwable e) { | ||
| if (stopWatch.isRunning()) { | ||
| stopWatch.stop(); | ||
| } | ||
|
|
||
| logFailure(methodInfo, e, stopWatch); | ||
| throw e; | ||
| } finally { | ||
| long totalTime = System.currentTimeMillis() - startTime; | ||
| MethodSignature signature = (MethodSignature) joinPoint.getSignature(); | ||
|
|
||
| log.info("[{}] {}.{} | Args: {} | Ret: {} | {}ms", | ||
| getLayer(signature), // 1. 레이어 (API / SVC) | ||
| className(signature), // 2. 클래스명 | ||
| methodName(signature), // 3. 메서드명 | ||
| formatArgs(joinPoint.getArgs()), // 4. 파라미터 (단순화) | ||
| formatResult(result), // 5. 결과값 (길이 제한) | ||
| totalTime // 6. 소요시간 | ||
| ); | ||
| } |
There was a problem hiding this comment.
예외 발생 시 로깅 기능이 완전히 제거되었습니다. 이전 코드에서는 try-catch 블록에서 예외를 캐치하여 logFailure 메서드로 상세한 에러 로그(클래스명, 메서드명, 파라미터, 예외 타입, 메시지, 소요시간)를 남겼습니다.
현재 코드는 finally 블록에서만 로깅하므로, 예외가 발생한 경우와 정상 실행된 경우를 구분할 수 없고, 예외 정보가 로그에 전혀 남지 않습니다.
GlobalExceptionAdvice에서 예외를 로깅하고 있지만, 이는 HTTP 요청 레벨의 예외만 처리하며, Service 계층에서 발생한 예외의 상세 컨텍스트(어떤 파라미터로 호출되었는지 등)를 추적하기 어렵습니다.
예외 발생 시 로그 레벨을 error로, 정상 실행 시 info로 구분하여 로깅하는 것을 권장합니다.
|
|
||
| @Pointcut("execution(* com.loopon..*Controller.*(..))") | ||
| public void controllerMethods() { | ||
| @Pointcut("execution(* com.loopon.*Controller.*(..))") |
There was a problem hiding this comment.
Pointcut 표현식이 변경되어 패키지 구조가 일치하지 않는 Controller들을 누락시킬 수 있습니다.
변경 전: "execution(* com.loopon..Controller.(..))" - 모든 하위 패키지의 Controller 포함
변경 후: "execution(* com.loopon.Controller.(..))" - 직접 하위 패키지의 Controller만 포함
실제 Controller들은 com.loopon.auth.presentation.AuthApiController, com.loopon.user.presentation.UserApiController 등 2단계 이상 깊은 패키지에 위치하므로, 현재 Pointcut은 어떤 Controller도 매칭하지 않습니다.
원래의 "..*Controller" 패턴으로 되돌려야 합니다.
| @Pointcut("execution(* com.loopon.*Controller.*(..))") | |
| @Pointcut("execution(* com.loopon..*Controller.*(..))") |
| @Test | ||
| @DisplayName("성공: 정상 수행 시 파라미터, 결과값, 소요시간이 로그에 남아야 한다") | ||
| void 정상_수행_로그_기록(CapturedOutput output) throws Throwable { | ||
| // Given | ||
| setupJoinPoint(UserCommandService.class, "signUp", | ||
| new String[]{"email", "nickname"}, | ||
| new Object[]{"test@test.com", "tester"}); | ||
|
|
||
| given(joinPoint.proceed()).willReturn("SuccessResult"); | ||
|
|
||
| // When | ||
| logAspect.logExecution(joinPoint); | ||
|
|
||
| // Then | ||
| verify(joinPoint, times(1)).proceed(); | ||
|
|
||
| assertThat(output.getOut()) | ||
| .contains("UserCommandService") | ||
| .contains("signUp") | ||
| .contains("email=test@test.com") | ||
| .contains("SuccessResult") | ||
| .containsPattern("\\[\\d+ms\\]"); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("마스킹: 민감한 키워드(password, token 등)는 ****로 가려져야 한다") | ||
| void 민감_정보_마스킹_처리(CapturedOutput output) throws Throwable { | ||
| // Given | ||
| setupJoinPoint(AuthService.class, "login", | ||
| new String[]{"email", "password", "refreshToken"}, | ||
| new Object[]{"user@test.com", "secret1234", "eyJh..."}); | ||
|
|
||
| given(joinPoint.proceed()).willReturn("LoginSuccess"); | ||
|
|
||
| // When | ||
| logAspect.logExecution(joinPoint); | ||
|
|
||
| // Then | ||
| assertThat(output.getOut()) | ||
| .contains("email=user@test.com") | ||
| .doesNotContain("secret1234") | ||
| .doesNotContain("eyJh...") | ||
| .contains("password=****") | ||
| .contains("refreshToken=****"); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("예외: 예외 발생 시 에러 로그가 남고 예외가 다시 던져져야 한다") | ||
| void 예외_발생_시_에러_로그_기록(CapturedOutput output) throws Throwable { | ||
| // Given | ||
| setupJoinPoint(UserQueryService.class, "getUser", | ||
| new String[]{"userId"}, | ||
| new Object[]{1L}); | ||
|
|
||
| given(joinPoint.proceed()).willThrow(new IllegalArgumentException("Invalid User ID")); | ||
|
|
||
| // When & Then | ||
| assertThatThrownBy(() -> logAspect.logExecution(joinPoint)) | ||
| .isInstanceOf(IllegalArgumentException.class) | ||
| .hasMessage("Invalid User ID"); | ||
|
|
||
| assertThat(output.getOut()) | ||
| .contains("[EXCEPTION]") | ||
| .contains("IllegalArgumentException") | ||
| .contains("Invalid User ID") | ||
| .contains("userId=1"); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("말줄임: 결과값이 100자를 넘으면 잘라서(...) 로깅해야 한다") | ||
| void 긴_결과값_말줄임_처리(CapturedOutput output) throws Throwable { | ||
| // Given | ||
| setupJoinPoint(BoardService.class, "getContent", new String[]{}, new Object[]{}); | ||
|
|
||
| String longString = "A".repeat(150); | ||
| @DisplayName("결과값이 100자를 넘으면 잘라야 한다") | ||
| void truncateResult() throws Throwable { | ||
| // given | ||
| String longString = "A".repeat(200); | ||
| given(joinPoint.proceed()).willReturn(longString); | ||
|
|
||
| // When | ||
| logAspect.logExecution(joinPoint); | ||
|
|
||
| // Then | ||
| assertThat(output.getOut()) | ||
| .contains(longString.substring(0, 100) + "...") | ||
| .doesNotContain(longString); | ||
| } | ||
|
|
||
| private void setupJoinPoint(Class<?> targetClass, String methodName, String[] paramNames, Object[] args) { | ||
| doReturn(targetClass).when(methodSignature).getDeclaringType(); | ||
| given(joinPoint.getSignature()).willReturn(signature); | ||
| given(signature.getDeclaringType()).willReturn(TestController.class); | ||
| given(signature.getName()).willReturn("testMethod"); | ||
| given(joinPoint.getArgs()).willReturn(new Object[]{}); | ||
|
|
||
| given(methodSignature.getName()).willReturn(methodName); | ||
| given(methodSignature.getParameterNames()).willReturn(paramNames); | ||
| // when | ||
| Object result = logAspect.logExecution(joinPoint); | ||
|
|
||
| given(joinPoint.getSignature()).willReturn(methodSignature); | ||
| given(joinPoint.getArgs()).willReturn(args); | ||
| // then | ||
| assertEquals(longString, result); | ||
| } |
There was a problem hiding this comment.
테스트 커버리지가 크게 축소되었습니다. 삭제된 중요 테스트 케이스:
- 정상_수행_로그_기록: 파라미터, 결과값, 소요시간 로깅 검증
- 민감_정보_마스킹_처리: password, token 등 마스킹 검증
- 예외_발생_시_에러_로그_기록: 예외 발생 시 로그 및 재throw 검증
현재는 결과값 말줄임 기능만 테스트하고 있으며, 이는 LogAspect의 핵심 기능인 메서드 실행 로깅과 예외 처리를 전혀 검증하지 않습니다.
기존 테스트들이 삭제된 이유가 해당 기능들이 제거되었기 때문이라면, 이는 기능 퇴보입니다. 최소한 기본적인 로깅 동작(메서드 호출 로그 생성, 파라미터 기록)에 대한 테스트는 필요합니다.
#⃣ 관련 이슈
Closes #161
📝 작업 내용
Check list
💬 리뷰 요구사항(선택)